Skip to content

Add coderabbit config and clean up configuration#2

Merged
adamwg merged 3 commits into
crossplane:mainfrom
adamwg:awg/config
May 6, 2026
Merged

Add coderabbit config and clean up configuration#2
adamwg merged 3 commits into
crossplane:mainfrom
adamwg:awg/config

Conversation

@adamwg

@adamwg adamwg commented May 5, 2026

Copy link
Copy Markdown
Member

Description of your changes

Three small changes to our repository configuration:

  1. Add a coderabbit config based on Crossplane's. We'll get coderabbit running on this repository once this is merged.
  2. Add @jcogilvie and @tampakrap to the CODEOWNERS file.
  3. Stop linting protobufs and pushing them to the Buf schema registry. Our protos are vendored from crossplane/crossplane where they are linted and pushed.

I have:

Summary by CodeRabbit

  • Chores

    • Updated code review automation configuration
    • Removed protobuf schema validation from CI workflow
    • Updated code ownership assignments
  • Documentation

    • Updated contributing guidelines with reference to official documentation

@adamwg adamwg requested a review from a team as a code owner May 5, 2026 22:10
@adamwg adamwg requested review from jbw976 and removed request for a team May 5, 2026 22:10
Copy Crossplane's coderabbit config as a starting point and remove irrelevant
parts.

Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>

@jbw976 jbw976 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple small comments, thanks @adamwg!

Comment thread .github/workflows/ci.yml Outdated
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6

- name: Lint and Push Protocol Buffers
- name: Lint Protocol Buffers

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd argue we don't need to do the protobuf linting - we also copy protobufs from c/c to https://github.com/crossplane/function-sdk-go and we don't lint them there either - so that has precedent 🤔

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes sense. Updated.

Comment thread .coderabbit.yaml
error testing uses proper patterns (cmpopts.EquateErrors, sentinel
errors for complex cases).

- path: "**/design/**"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see some patterns that are in c/c but not applicable here - want to take a pass to trim this down to only what's applicable in this repo?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apis/ and test/ I expect will be relevant in the near future, so I left them in. design/ I'm less certain about but figured it was worth keeping in case we introduce design docs here as well.

Comment thread CODEOWNERS

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was required by the policy. In any case I'd say to keep it, it allows us also to automatically assign people on PRs and also we might want to match dirs with people in the future

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you referring to the community extension project policies?

just to be clear, the only requirement there is for OWNERS.md, there is no requirement for CODEOWNERS

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the CODEOWNERS to include the additional maintainers rather than removing it completely.

adamwg added 2 commits May 6, 2026 16:32
This repository has additional maintainers beyond the
@crossplane/crossplane-maintainers group.

Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
The protos in this repository are vendored from crossplane/crossplane, so no
need to lint them or push them to the Buf schema registry. This avoids the need
for a `BUF_TOKEN` secret in this repo.

Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Warning

Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories.

📝 Walkthrough

Walkthrough

A comprehensive CodeRabbit review configuration is introduced alongside adjustments to CI workflow, code ownership assignments, and contributing documentation. The protobuf schema linting CI job is removed, and two additional maintainers are added to the default code-owner rule.

Changes

Repository Governance & Review Configuration

Layer / File(s) Summary
Review Configuration
.coderabbit.yaml
Introduces a new CodeRabbit configuration defining review language, path-specific review instructions for Go, tests, APIs, CLIs, and docs, pre-merge checks (title length, breaking changes, feature-gate requirements), and disables external tooling checks (buf, golangci-lint, gitleaks, semgrep, yamllint, etc.).
CI Workflow Adjustment
.github/workflows/ci.yml
Removes the protobuf-schemas job that performed protobuf linting via bufbuild/buf-action, aligning with CodeRabbit configuration that disables buf-based analysis.
Code Ownership
CODEOWNERS
Adds two additional maintainers (jcogilvie and tampakrap) to the default code-owner assignment for all files.
Documentation
CONTRIBUTING.md
Replaces content with a link to Crossplane's contributing documentation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 The warren grows with reviewers new,

CodeRabbit hops to check what's true,

Protobuf paths are cleared away,

Config and governance find their way! 🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main changes: adding a coderabbit configuration file and cleaning up configuration files (CODEOWNERS, CI workflow, CONTRIBUTING).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.coderabbit.yaml:
- Around line 170-173: Remove the stray leading double-quote at the start of the
block scalar for the custom check description that begins with "Fails if files
under 'apis/**' or 'cmd/**'..." so the text is a normal YAML block (no literal
leading quote); locate the YAML value containing that sentence and delete the
initial " character so the check instructions read cleanly without the preserved
quote.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 73f5b6c1-4ea8-4859-89ea-e461c0ce4e4e

📥 Commits

Reviewing files that changed from the base of the PR and between f4564b3 and 40224e7.

📒 Files selected for processing (4)
  • .coderabbit.yaml
  • .github/workflows/ci.yml
  • CODEOWNERS
  • CONTRIBUTING.md
💤 Files with no reviewable changes (1)
  • .github/workflows/ci.yml

Comment thread .coderabbit.yaml
Comment on lines +170 to +173
"Fails if files under 'apis/**' or 'cmd/**' remove or rename public
fields/flags, add new required public fields/flags, or remove behavior
without label 'breaking-change'.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove the stray leading quote in custom check instructions.

The block scalar begins with an extra " character, which is preserved literally and makes the check text noisy/ambiguous.

Suggested fix
       - name: "Breaking Changes"
         mode: "error"
         instructions: |
-          "Fails if files under 'apis/**' or 'cmd/**' remove or rename public
+          Fails if files under 'apis/**' or 'cmd/**' remove or rename public
           fields/flags, add new required public fields/flags, or remove behavior
           without label 'breaking-change'.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"Fails if files under 'apis/**' or 'cmd/**' remove or rename public
fields/flags, add new required public fields/flags, or remove behavior
without label 'breaking-change'.
- name: "Breaking Changes"
mode: "error"
instructions: |
Fails if files under 'apis/**' or 'cmd/**' remove or rename public
fields/flags, add new required public fields/flags, or remove behavior
without label 'breaking-change'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.coderabbit.yaml around lines 170 - 173, Remove the stray leading
double-quote at the start of the block scalar for the custom check description
that begins with "Fails if files under 'apis/**' or 'cmd/**'..." so the text is
a normal YAML block (no literal leading quote); locate the YAML value containing
that sentence and delete the initial " character so the check instructions read
cleanly without the preserved quote.

@adamwg adamwg merged commit d7a5edf into crossplane:main May 6, 2026
10 checks passed
@adamwg adamwg deleted the awg/config branch May 6, 2026 22:48
jcogilvie added a commit to jcogilvie/cli that referenced this pull request Jun 2, 2026
Reworks the validate package to address eight comments from the PR
review pass.

Renderer: polymorphic dispatch
* render.Renderer is now an interface; textRenderer, jsonRenderer, and
  yamlRenderer each implement it as their own concrete type. Adding a
  new format means adding a Renderer and registering it in the
  renderers map — no switch on string. (review crossplane#3, crossplane#4)
* OutputFormat.Render dispatches via the registry; the empty string
  defaults to text for safety of zero-value callers.
* renderText handles all per-resource line emission inline; the
  per-error prefix selection is encapsulated in a private helper that
  does not pretend to handle the [✓] or "[!] could not find" cases.

SchemaValidate: decomposed
* Extracted validateResource as a per-resource helper. SchemaValidate's
  body is now a clean fan-out and the //nolint:gocognit suppression is
  gone. (review crossplane#2)

Tests: tightened, idiomatic, end-to-end
* TestSchemaValidate now asserts on the full []FieldValidationError
  per resource (Type + Field), with cmpopts.IgnoreFields hiding the
  Message and Value strings that drift across k8s validator versions.
  Dropped the bespoke containsAllErrorTypes "at least once" matcher.
  (review crossplane#7)
* All tests that take a context now use t.Context() instead of
  context.Background(). (review crossplane#6)
* render text tests no longer compare full output bytes against a
  string constant; they assert on line counts plus per-line substrings
  with a generated summary line. (review crossplane#5)
* Dropped the validateAndRender helper on Cmd. The validation +
  rendering + error-shaping logic lives directly in Cmd.Run.
  cmd_test.go now drives the command end-to-end through kong.Parse +
  ctx.Run against real YAML fixtures, capturing stdout from a real
  kong.Context. To keep the e2e run offline, testdata/cache contains a
  stand-in crossplane image package.yaml and the tests pass a matching
  --crossplane-image. (review #1, crossplane#8)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants